New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Remove all client side validation for OLM, allow nonspecific lif… #1160
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Jesse
* expressed as subclasses of this class, accessed by static factory methods. | ||
*/ | ||
public abstract static class LifecycleAction implements Serializable { | ||
public static class LifecycleAction implements Serializable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to remove the modifier abstract in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Abstract classes can't be instantiated so we do if we want to allow for unsupported action types to be created in this way.
The alternative would be to make an implementation of LifecycleAction for that purpose, like "CustomLifecycleAction" or something, but I'm worried that doing that would be confusing, or imply that this is the correct way to use action types that aren't in the library (when in actuality users should upgrade to the latest version of the library)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, totally missed that. Thanks Jesse, I would confirm what happens when a say setStorageClass doesn't have storageClass in the rule (the case an action type is added that requires additional parameters) and see how GCS handles the request on a PATCH request.
I suspect it will fail; so we may want to clarify if we want the library not to create an instance for that unknown action type.
@@ -548,8 +548,12 @@ static LifecycleRule fromPb(Rule rule) { | |||
StorageClass.valueOf(action.getStorageClass())); | |||
break; | |||
default: | |||
throw new UnsupportedOperationException( | |||
"The specified lifecycle action " + action.getType() + " is not currently supported"); | |||
log.warning( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add tests to ensure unknown types no longer throw exceptions?
google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java
Show resolved
Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java
Outdated
Show resolved
Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java
Outdated
Show resolved
Hide resolved
@JesseLovelace are you still addressing the clirr issue? |
public abstract String getActionType(); | ||
private final String actionType; | ||
|
||
private LifecycleAction(String actionType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this constructor public
to allow someone (as remote a chance as it is) that is already extending themselves allow them to continue to extend, but now they would need to provide the actionType
.
/cherry-pick v2.1.9 |
/cherry-pick 2.1.x |
#1160) * fix: Remove all client side validation for OLM, allow nonspecific lifecycle actions * Test unsupported actions * address comments * Fix clirr * 🦉 Updates from OwlBot See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * checkstyle Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
* fix: Remove all client side validation for OLM, allow nonspecific lifecycle actions * Test unsupported actions * address comments * Fix clirr * 🦉 Updates from OwlBot See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * checkstyle Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> Co-authored-by: JesseLovelace <43148100+JesseLovelace@users.noreply.github.com> Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Removes client side validation by refactoring
LifecycleAction
into a non-abstract class and allowing for nonspecific lifecycle actions.